-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
probe-cli: Update to quic-go v0.37.3 #1161
Conversation
@kelmenhorst there are several merge conflicts, could you make sure they are resolved? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐳
However, it seems we probably need to do more work either in this pull request or elsewhere. Here's how tests fail for me locally:
I need to figure out what's the issue here. Here's how to reproduce by only running the package that fails:
IIUC, there is some sort of "global" state here? |
This diff fixes a bug in the webconnectivityqa tests which we discovered due to the new quic-go API panicking (#1161 (comment)). The bug was that we did not close the `QAEnv` in some of our webconnectivityqa tests. Therefore, the QUIC Server would not close the underlying UDP socket (`net.PacketConn`) and the local address of this socket would not be removed from the QUIC multiplexer. In the subsequent test case, the new quic-go API (see #1161) would panic because the local address of the server would still be registered in the global QUIC multiplexer.
This diff fixes a bug in the webconnectivityqa tests which we discovered due to the new quic-go API panicking (ooni#1161 (comment)). The bug was that we did not close the `QAEnv` in some of our webconnectivityqa tests. Therefore, the QUIC Server would not close the underlying UDP socket (`net.PacketConn`) and the local address of this socket would not be removed from the QUIC multiplexer. In the subsequent test case, the new quic-go API (see ooni#1161) would panic because the local address of the server would still be registered in the global QUIC multiplexer.
## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2492 ## Description This diff updates our QUIC code, mainly in netxlite, to quic-go v0.37.1, including the new API introduced in v0.35.0. The migration has been done according to ooni/probe#2492. Major changes: * Replaced call to `quic.DialEarlyContext` with call to `quic.DialEarly` (netxlite/quic.go). * Removed now unnecessary address string in `DialEarly` calls (netxlite/quic.go). * Replaced `quic.Listener` with `*quic.Listener` (simplequicping_test.go). * Adapted signature of `HandshakeComplete` such that it returns a channel rather than a context (quic code in netxlite, measurex, mocks). * Renamed `QUICListener` to `UDPListener`. * `quic.ConnectionState.TLS` points directly to a `tls.ConnectionState` instead of pointing to a `qtls.ConnectionState` instance. Background: Starting with Go 1.21, quic-go will use the QUIC support provided by the standard library's TLS implementation. Thus, quic-go will no longer fork crypto/tls anymore. * changed signature of `quic.Connection`'s `ReceiveMessage` method --------- Co-authored-by: Simone Basso <[email protected]>
Checklist
Description
This diff updates our QUIC code, mainly in netxlite, to quic-go v0.35.1, including the new API introduced in v0.35.0.
The migration has been done according to ooni/probe#2492.
Major changes:
quic.DialEarlyContext
with call toquic.DialEarly
(netxlite/quic.go).DialEarly
calls (netxlite/quic.go).quic.Listener
with*quic.Listener
(simplequicping_test.go).HandshakeComplete
such that it returns a channel rather than a context (quic code in netxlite, measurex, mocks).QUICListener
toUDPListener
.Attention: Tests using a failingMockSyscallConn
(e.g. in measurexlite) fail, due to issues with closing the quic.Transport quic-go.Update (07.08.2023):
Upgraded to quic-go 0.37.3 with the following changes:
quic.ConnectionState.TLS
points directly to atls.ConnectionState
instead of pointing to aqtls.ConnectionState
instance. Background: Starting with Go 1.21, quic-go will use the QUIC support provided by the standard library's TLS implementation. Thus, quic-go will no longer fork crypto/tls anymore.quic.Connection
'sReceiveMessage
method